Automated Test: sms-retry-enhanced #380
Conversation
* add retry count to workflow reminder * add logic to for retry count --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com> Co-authored-by: Udit Takkar <53316345+Udit-takkar@users.noreply.github.com>
📝 WalkthroughWalkthroughThis pull request introduces a retry count mechanism for workflow SMS reminders. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts`:
- Around line 189-198: The catch block that calls prisma.workflowReminder.update
can itself throw and abort the enclosing for loop; wrap the retryCount update in
its own try-catch (or append a .catch()) so failures to increment
reminder.retryCount are swallowed/logged and do not rethrow. Specifically,
protect the prisma.workflowReminder.update call that uses reminder.id and
reminder.retryCount inside scheduleSMSReminders.ts (inside the outer catch) with
a nested try/catch (or use prisma.workflowReminder.update(...).catch(...)) and
log the update error without throwing so the loop continues processing remaining
reminders.
- Around line 31-43: The OR branch currently deletes reminders with retryCount >
1 regardless of method; update the condition so the second alternative also
requires method === WorkflowMethods.SMS (i.e., make the OR array entry include
both { method: WorkflowMethods.SMS, retryCount: { gt: 1 } }) so only SMS
reminders are matched for the retryCount-based deletion; modify the OR clause
around WorkflowMethods.SMS, scheduledDate and retryCount to include this method
filter.
| OR: [ | ||
| { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| }, | ||
| { | ||
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if retryCount is used/incremented by other workflow method handlers (email, whatsapp)
rg -n "retryCount" --type=ts -C 3Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 4246
🏁 Script executed:
#!/bin/bash
# Search for other workflow reminder handlers
fd -e ts -path "*/workflows/*" "schedule.*Reminders"
echo "---"
# Search for WorkflowReminder model/schema definition
fd -e ts -o -e prisma | xargs rg -l "WorkflowReminder" | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1793
🏁 Script executed:
#!/bin/bash
# Check if email and whatsapp handlers use retryCount
echo "=== WHATSAPP HANDLER ==="
rg -n "retryCount" packages/features/ee/workflows/api/scheduleWhatsappReminders.ts
echo ""
echo "=== EMAIL HANDLER ==="
rg -n "retryCount" packages/features/ee/workflows/api/scheduleEmailReminders.ts
echo ""
echo "=== MIGRATION FILE ==="
cat packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sqlRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 262
🏁 Script executed:
#!/bin/bash
# Get the full WHERE clause context to understand the delete operation
sed -n '25,50p' packages/features/ee/workflows/api/scheduleSMSReminders.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 657
🏁 Script executed:
#!/bin/bash
# Get more context around the delete operation including what happens after
sed -n '28,65p' packages/features/ee/workflows/api/scheduleSMSReminders.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 961
🏁 Script executed:
#!/bin/bash
# Check the catch block and surrounding code to verify the update error handling
sed -n '165,200p' packages/features/ee/workflows/api/scheduleSMSReminders.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 917
Add method filter to retryCount deletion condition to prevent unintended removal of non-SMS reminders.
The second OR branch deletes any WorkflowReminder with retryCount > 1, regardless of method. While currently only the SMS handler increments retryCount (keeping email/WhatsApp reminders at 0), this is a logic error that could cause issues if retry logic is added to other methods in the future.
Proposed fix
OR: [
{
method: WorkflowMethods.SMS,
scheduledDate: {
lte: dayjs().toISOString(),
},
},
{
+ method: WorkflowMethods.SMS,
retryCount: {
gt: 1,
},
},
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| OR: [ | |
| { | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| }, | |
| { | |
| retryCount: { | |
| gt: 1, | |
| }, | |
| }, | |
| ], | |
| OR: [ | |
| { | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| }, | |
| { | |
| method: WorkflowMethods.SMS, | |
| retryCount: { | |
| gt: 1, | |
| }, | |
| }, | |
| ], |
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts` around lines 31 -
43, The OR branch currently deletes reminders with retryCount > 1 regardless of
method; update the condition so the second alternative also requires method ===
WorkflowMethods.SMS (i.e., make the OR array entry include both { method:
WorkflowMethods.SMS, retryCount: { gt: 1 } }) so only SMS reminders are matched
for the retryCount-based deletion; modify the OR clause around
WorkflowMethods.SMS, scheduledDate and retryCount to include this method filter.
| } catch (error) { | ||
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, | ||
| }); | ||
| console.log(`Error scheduling SMS with error ${error}`); |
There was a problem hiding this comment.
The prisma.update in the catch block can throw and abort processing of remaining reminders.
If the retryCount update on Line 190 fails (e.g., DB connectivity issue), the error propagates out of the catch block and the for loop, skipping all subsequent unscheduled reminders and potentially returning a 500 instead of the 200 on Line 201.
Wrap this in a nested try-catch or use .catch() to ensure the loop continues.
🛡️ Proposed fix
} catch (error) {
- await prisma.workflowReminder.update({
- where: {
- id: reminder.id,
- },
- data: {
- retryCount: reminder.retryCount + 1,
- },
- });
+ try {
+ await prisma.workflowReminder.update({
+ where: {
+ id: reminder.id,
+ },
+ data: {
+ retryCount: reminder.retryCount + 1,
+ },
+ });
+ } catch (updateError) {
+ console.log(`Error updating retryCount for reminder ${reminder.id}: ${updateError}`);
+ }
console.log(`Error scheduling SMS with error ${error}`);
}🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts` around lines 189
- 198, The catch block that calls prisma.workflowReminder.update can itself
throw and abort the enclosing for loop; wrap the retryCount update in its own
try-catch (or append a .catch()) so failures to increment reminder.retryCount
are swallowed/logged and do not rethrow. Specifically, protect the
prisma.workflowReminder.update call that uses reminder.id and
reminder.retryCount inside scheduleSMSReminders.ts (inside the outer catch) with
a nested try/catch (or use prisma.workflowReminder.update(...).catch(...)) and
log the update error without throwing so the loop continues processing remaining
reminders.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Improvements